Unified state directory + bbolt settings store - #45

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.gitignore:
- Line 61: The .gitignore entry "omniview" is unanchored and will match that
name anywhere in the tree; update the pattern to anchor it to the repository
root by replacing "omniview" with "/omniview" (or "/omniview/" if you want to
explicitly indicate a directory) so only the root-level compiled
binary/directory is ignored.

In `@backend/clients/logger.go`:
- Around line 31-33: The os.MkdirAll(logDir, 0755) error is being swallowed by
immediately returning zap.L().Sugar(); instead, capture the error, emit a clear
error message that includes the logDir and the error (e.g., via zap.L().Error or
fmt.Fprintf(os.Stderr, ...)), and then fall back to returning the default
logger; replace the bare return zap.L().Sugar() in the os.MkdirAll error branch
with code that logs the failure context (path and err) before returning
zap.L().Sugar().
- Line 35: Replace the use of path.Join when building local filesystem paths:
change the expression that assigns logFile (logFile := path.Join(logDir,
"app.log")) to use filepath.Join instead and update imports to include
"path/filepath" (removing "path" if it's no longer used) so the value passed
into lumberjack.Logger.Filename is an OS-native filesystem path.

In `@backend/pkg/plugin/devserver/ports.go`:
- Around line 127-132: The ReadFile error in PortAllocator.CleanupStaleProcesses
is treated as "no PID file" for all errors; change the handling after
pa.stateRoot.ReadFile(devserverPIDFileName) to check specifically for a missing
file (using os.IsNotExist(err) or errors.Is(err, fs.ErrNotExist)) and return
silently only in that case, but for any other error log/return the unexpected
read error via the provided logger (or propagate it) so permission/I/O issues
aren't swallowed; update the CleanupStaleProcesses function and references to
pa.stateRoot.ReadFile and devserverPIDFileName accordingly.

In `@backend/pkg/plugin/manager.go`:
- Around line 362-388: The initial os.ReadDir(pluginDir) happens before the
reverse-migration loop, so after renaming dev folders (pm.pluginsRoot.Rename)
the files slice is stale and the later load pass may still iterate old names;
fix by re-reading the directory (call os.ReadDir(pluginDir) again and update the
files variable) after the loop that checks states and performs Rename (or move
the initial scan to after that migration block) so subsequent code that relies
on files/stateByID sees the migrated canonical directories (refer to
variables/functions: files, os.ReadDir, states, state.ID,
pm.pluginsRoot.Rename).

In `@backend/pkg/plugin/pids.go`:
- Around line 118-122: The helper isNotExist is unused; update CleanupStale to
use it for clearer error handling: replace any direct os.IsNotExist(err) checks
or inline error comparisons in CleanupStale with calls to isNotExist(err) and
ensure the cleanup logic treats "not exist" as a non-fatal/expected condition
(skip or continue without logging as an error) while other errors are
handled/logged as before; alternatively if you prefer not to add that usage,
delete the unused isNotExist function (keeping CleanupStale behavior unchanged).

In `@backend/pkg/plugin/settings/controller.go`:
- Around line 134-195: hydratePlugin can return early on failures (e.g.,
client.SetSettings) leaving callers blocked on pendingChanges; add a helper
method failPendingChanges(pluginID string, err error) that grabs pendingMu,
removes c.pendingChanges[pluginID], then iterates the slice sending err on each
p.errCh, and call this helper in hydratePlugin error paths (when schema == nil
and when client.SetSettings returns an error) before setting
c.hydrated[pluginID]=false and returning so all queued SetSetting/SetSettings
callers are signaled; reference pendingChanges, pendingMu, failPendingChanges,
hydratePlugin, SetSettings, and drainPendingChanges.

In `@backend/pkg/store/utils.go`:
- Line 21: Change the file mode used when creating/opening the store data file
to avoid giving execute permission; replace the current 0755 mode in the
root.OpenFile(store, os.O_RDWR|os.O_CREATE, 0755) call with a non-executable
mode such as 0644 (or 0600 for sensitive data) so the file is created with
proper data-file permissions.

In `@cmd/omniview-plugin-dev/devinfo.go`:
- Around line 23-38: The resolver resolveStateDir currently only checks
OMNIVIEW_STATE_DIR and the user home; add support for a build-time override
variable (e.g., a package-level string set via ldflags such as stateDirOverride)
with precedence env > ldflags > default. In resolveStateDir, after checking
OMNIVIEW_STATE_DIR but before falling back to the home dir, if stateDirOverride
is non-empty validate it is absolute (use filepath.IsAbs), return a cleaned
path, and surface an error if it's not absolute; keep existing behavior for the
env var and default otherwise.

In `@docs/superpowers/plans/2026-03-20-unified-appstate.md`:
- Line 9: The tech stack line incorrectly states Go 1.26 for os.OpenRoot and
os.Root; update that line to reference the correct minimum Go version (Go 1.24)
and keep the symbols os.OpenRoot and os.Root in the description so readers know
which APIs require the version bump.
- Around line 1784-1793: The fenced code blocks containing the ldflags for
buildStateDir (the lines with "-X
github.com/omniviewdev/omniview/internal/appstate.buildStateDir=~/.omniview-nightly"
and "-X
github.com/omniviewdev/omniview/internal/appstate.buildStateDir=~/.omniview-pr")
need a language specifier; update each triple-backtick fence to include a
language token (e.g., use ```text or ```shell) so the blocks are consistent with
the rest of the document and render correctly; leave the content of the blocks
unchanged and ensure the surrounding text (mention of pr.yml and the Step 2
checklist) remains intact.

In `@docs/superpowers/specs/2026-03-20-unified-appstate-design.md`:
- Around line 254-256: The fenced code block containing the ldflags example
currently lacks a language specifier; update the triple-backtick fence to
include "text" so the block reads ```text and preserves the literal ldflag
string -X
github.com/omniviewdev/omniview/internal/appstate.buildStateDir=<value> for
proper rendering.
- Around line 328-351: The code block showing the directory tree lacks a
language specifier; update the fenced block around the "~/.omniview/" tree (the
block that currently starts with triple backticks) to include "text" (i.e.,
replace the opening ``` with ```text and keep the closing ```), so the directory
layout renders correctly as plain text.
- Around line 47-54: The fenced code block that lists the internal/appstate
package files lacks a language specifier; update the opening fence from ``` to
include a specifier (e.g., ```text) so the block renders correctly —
specifically change the block that begins with "internal/appstate/" and contains
lines like "appstate.go // Service, New(), Close(), Options" to use a language
tag.

In `@go.mod`:
- Line 100: The go.mod entry for the import "go.etcd.io/bbolt" is marked //
indirect but the package is directly imported in the codebase (import
"go.etcd.io/bbolt"), so update go.mod to make this a direct dependency: remove
the "// indirect" marker for the go.etcd.io/bbolt v1.4.3 line (or run `go get
go.etcd.io/bbolt@v1.4.3` to update module files) so the module correctly records
bbolt as a direct dependency.

In `@internal/appstate/appstate.go`:
- Around line 137-155: PluginData and PluginStore allocate a new ScopedRoot on
every call (via newScopedRoot and fallback &ScopedRoot{path: dir}), leaking
descriptors and losing containment on errors; instead cache per-plugin
ScopedRoot instances on the Service (keyed by plugin id) and return the cached
instance on subsequent PluginData/PluginStore calls, create them once using
newScopedRoot and store them in a map field on Service, ensure Service.Close
iterates the cache to call Close on each ScopedRoot, and remove or replace the
unsafe fallback (&ScopedRoot{path: dir}) so errors are propagated or handled by
returning nil/error or creating and caching a proper ScopedRoot only on success.

In `@internal/appstate/resolve_test.go`:
- Around line 27-37: Test mutates the package-level buildStateDir, which races
under parallel tests; update the test to avoid global mutation or ensure
per-test cleanup via t.Cleanup. Specifically, in TestResolveRoot_Default replace
the manual save/restore with t.Cleanup(func(){ buildStateDir = old }) (or
better, refactor ResolveRoot to accept an injected stateDir or add a test-only
setter like SetBuildStateDir and use t.Cleanup to reset it) so that
buildStateDir is reliably restored even when tests run in parallel; reference
buildStateDir, ResolveRoot, and TestResolveRoot_Default when making the change.

In `@internal/appstate/scoped_test.go`:
- Around line 147-152: In TestScopedRoot_Rename_RejectsTraversal, the call to
r.WriteFile("safe.txt", ...) currently discards the error; change it to assert
the setup succeeded by replacing the `_ = r.WriteFile(...)` with
`require.NoError(t, r.WriteFile("safe.txt", []byte("data"), 0644))` so the test
fails immediately on write errors and doesn’t proceed to r.Rename in an invalid
state; keep the rest of the test (newTestScopedRoot, r.Rename, assert.Error)
unchanged.

In `@internal/appstate/scoped.go`:
- Around line 38-49: The validate function currently allows an empty string; add
an explicit check in validate(name string) to reject "" (e.g., if name == ""
return fmt.Errorf("appstate: empty name not allowed")) so callers get a clear
error instead of OS-level failures; update the validate function's error path
(used by ReadFile and other appstate methods) to return this descriptive error
for empty input.
- Around line 86-91: RemoveAll currently calls os.RemoveAll on
filepath.Join(s.path, name), which creates a TOCTOU race and allows deleting the
scoped root when name is empty; instead implement RemoveAll by rejecting
empty/"." names and recursively enumerating the target directory via the
contained root methods (e.g., use s.root.ReadDir on filepath.Join(s.path,
filepath.FromSlash(name)) to list entries and call s.root.Remove on each entry),
performing all operations through s.root so kernel-enforced containment is
preserved and re-validating each child name before removal to avoid race
windows; refer to the ScopedRoot.RemoveAll function and the contained root
methods (s.root.ReadDir, s.root.Remove) when making the change.
- Around line 124-126: ResolvePath currently skips containment checks and can
return absolute or escaped paths; change its signature to ResolvePath(name
string) (string, error), call s.validate() at the start (propagate the error)
and only on success return filepath.Join(s.path, filepath.FromSlash(name));
update callers to handle the returned error so fsnotify/lumberjack receive
validated, contained paths.

In `@internal/settings/store/migrate.go`:
- Around line 152-166: The renameToBackup function can overwrite .gob.bak.100 if
all numbered backups 1–100 exist; change renameToBackup to detect when no free
candidate was found (after the for loop) and handle it by returning a clear
error (or generating a unique name, e.g., append timestamp/UUID) instead of
proceeding to os.Rename; make the check around the loop that sets backup and, if
no available candidate was found, return an error indicating no free backup slot
(or fallback to a unique suffix) so renameToBackup never blindly overwrites an
existing file.

In `@internal/settings/store/store.go`:
- Around line 37-47: The bolt.Open failure handling currently treats all open
errors as corruption and renames the DB; change this to only perform corruption
recovery for bbolt corruption errors by adding an isCorruptionOpenError(err
error) bool helper that returns true for bbolt.ErrInvalid,
bbolt.ErrVersionMismatch, and bbolt.ErrChecksum (match bbolt v1.4.3 exported
errors), and otherwise propagate the original error. Update the store
initialization flow (the bolt.Open call and subsequent rename/reopen logic) to
call isCorruptionOpenError(err) before renaming the file and attempting
recovery; if the helper returns false, return the original err unchanged. Ensure
references use bolt.Open and the new isCorruptionOpenError helper so behavior is
narrowly scoped to true on-disk corruption cases only.

In `@main.go`:
- Around line 91-102: Seed the telemetry configuration from the persisted
settings before initializing the telemetry service: call
settingsStore.LoadCategory("telemetry") (checking settingsStore != nil and
handling load error) and merge or override telemetry.DefaultConfig(...) with the
loaded telemetry category into the telemetryCfg variable before
constructing/initializing telemetrySvc and calling telemetrySvc.Init(); do the
same wherever telemetry.DefaultConfig()/telemetrySvc.Init() are used (e.g., the
other occurrences noted) so startup uses persisted disable/endpoints/auth values
rather than the later callback from settingsstore.MigrateFromGOB.

---

Outside diff comments:
In `@backend/pkg/plugin/devserver/gowatch.go`:
- Line 377: Update the outdated comment that refers to an absolute home path;
replace the hardcoded "~/.omniview/plugins/<id>/bin/plugin" text with the new
scoped-root form using the pluginsRoot abstraction (e.g.,
"<pluginsRoot>/<pluginID>/bin/plugin") so it matches the implementation in
gowatch.go (reference symbols: pluginsRoot, pluginID).

In `@backend/pkg/plugin/devserver/integration_test.go`:
- Around line 199-207: Replace the nil ScopedRoot argument passed into the
integration constructors (e.g., newGoWatcherProcess) with an explicit test
ScopedRoot from appstate.NewTestService(t).RootDir() so tests are hermetic;
locate calls like newGoWatcherProcess(..., nil, ...) and other similar
constructor invocations and inject appstate.NewTestService(t).RootDir() (or the
equivalent ScopedRoot) in place of nil to ensure unified-state path is explicit
across all listed test call sites.

In `@backend/pkg/plugin/settings/controller.go`:
- Around line 255-265: OnPluginStop currently sets hydrated[pluginID]=false but
leaves pending changes around; update controller.OnPluginStop to also clear any
pending changes for the plugin (the same cleanup performed in OnPluginDestroy)
while holding c.mu: remove entries from c.pendingChanges[pluginID] (or delete
the map key) and ensure c.clients and c.hydrated are updated inside the same
mutex-protected block to avoid races when a plugin restarts.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@backend/pkg/plugin/settings/controller.go`:
- Around line 546-553: SetSetting/SetSettings currently blocks on reading from
errCh (pendingChanges map update uses pendingMu and returns <-errCh), which can
hang the caller if hydration is slow; modify these functions to accept a
context.Context (or use a timeout) and replace the blocking receive with a
select that returns either the error from errCh or ctx.Done() (e.g., select {
case err := <-errCh: return err case <-ctx.Done(): return ctx.Err() }); update
all call sites to pass a context and apply the same pattern for the other
identical block that reads from errCh (the second occurrence around
pendingChanges usage).
- Around line 348-365: bboltFallbackSettings currently recreates
pkgsettings.Setting with only ID and Value from c.store.LoadPluginSettings;
update it to also populate Label, Description, Type, Default, Options and
validation fields by looking up the plugin's setting schema/definitions held on
the controller (e.g., the controller's plugin-setting schema cache or loader)
and merging those metadata fields into each pkgsettings.Setting; if no schema
exists for a setting, populate reasonable defaults (e.g., Type="string", empty
Options/Validation) so UI components receive full metadata during fallback.

In `@cmd/omniview-plugin-dev/devinfo.go`:
- Around line 28-30: Update the docstring for resolveStateDir to reflect the
three-tier priority used in the implementation: OMNIVIEW_STATE_DIR env var
first, then the stateDirOverride parameter, and finally the default ~/.omniview;
mention the middle tier by name (stateDirOverride) and the exact order so the
comment matches the actual behavior in resolveStateDir.
- Around line 32-43: The environment and flag path handling in Get
(OMNIVIEW_STATE_DIR and stateDirOverride) rejects tilde-prefixed paths because
it uses filepath.IsAbs() directly; make it consistent with the main app by
reusing or mirroring expandAndValidate/ResolveRoot behavior: import and call the
same expandAndValidate (or implement equivalent tilde expansion + validation)
before checking IsAbs(), then return filepath.Clean of the expanded path; update
the logic that references OMNIVIEW_STATE_DIR and stateDirOverride so tilde (~)
is expanded and validated the same way ResolveRoot() does.

In `@internal/appstate/appstate.go`:
- Around line 125-144: Service.Close currently ignores errors from multiple
Close calls; change it to collect each returned error (from r.Close() on entries
in pluginDataRoots and pluginStoreRoots, and from s.plugins.Close(),
s.logs.Close(), s.rootDir.Close(), s.osRoot.Close(), and releaseLock(s.lock))
into a slice and use errors.Join to aggregate and return the combined error.
Acquire pluginRootsMu as now while iterating, still clear slices and unlock, but
for every Close() call check the error and append non-nil errors to a list;
after all closes complete, return errors.Join(allErrors...) (or nil when none)
instead of always returning nil. Ensure you import "errors" and keep the
existing order of cleanup and releaseLock(s.lock) while including its error in
the aggregation.

In `@internal/appstate/resolve_test.go`:
- Line 20: The tests call os.UserHomeDir() and ignore its error return, which
can make assertions misleading; update each occurrence (the calls assigning to
home) to capture the returned error and assert it with require.NoError(t, err)
before using home (e.g., in resolve_test.go near the assignments at the
instances on lines referenced), and similarly adjust any helper/variable
initializations that use UserHomeDir() so they check err first; ensure you
import testify/require if not already present.

In `@internal/appstate/scoped_test.go`:
- Around line 131-145: Add a new unit test named
TestScopedRoot_RemoveAll_SymlinkEscape that uses newTestScopedRoot to create a
target directory outside the scope and a symlink inside the scoped root that
points to that external directory, then call RemoveAll on the scoped path
containing the symlink and assert RemoveAll returns an error (and that the
external directory remains untouched via Stat); reference the scoped RemoveAll
implementation to ensure the test triggers the TOCTOU symlink escape scenario
and verifies the guarded behavior.

In `@internal/appstate/scoped.go`:
- Around line 89-100: The RemoveAll method currently validates the name but
still has a TOCTOU symlink escape risk between validate() and os.RemoveAll();
update ScopedRoot.RemoveAll to resolve the final removal path with
filepath.EvalSymlinks (or similar) on filepath.Join(s.path,
filepath.FromSlash(name)), then verify the resolved path is still contained
under s.path before calling os.RemoveAll to prevent symlink escapes, returning
an error if containment fails; reference validate, ScopedRoot.RemoveAll,
os.RemoveAll, and filepath.EvalSymlinks when making the change.

In `@internal/settings/store/migrate.go`:
- Around line 70-74: The warning for corrupt files in the decodeGOB error path
is toosilent; change the migration flow (in migrate.go around the decodeGOB call
and the similar block at the other location) to record skipped corrupt file
paths instead of simply returning nil, e.g., append path and error info to a
local slice (e.g., skippedFiles or skippedSettings) and continue processing;
after the migration loop completes (in the same function that orchestrates these
decodes), emit a single aggregated log entry summarizing all skipped files and
their errors so users can see which settings were dropped during migration
(refer to decodeGOB and the surrounding migration function where the warnings
are currently logged).

In `@main.go`:
- Around line 91-102: The current code silently continues when
settingsstore.Open fails, losing persistence; change this to set a clear
"persistence disabled" state and surface that to the rest of the app instead of
silently proceeding. Specifically, when settingsstore.Open(...) returns an
error, set a package-level or context-scoped flag (e.g., persistenceEnabled =
false or appState.PersistenceDisabled) and emit a higher-severity log (use
log.Printf or your app logger) describing the failure; ensure callers that rely
on settingsStore (the settingsStore variable) check the flag or nil before
attempting writes/migrations and the UI/CLI reads the flag to display a
degraded-mode indicator to the user. If you prefer to abort instead, replace the
current stderr-only handling with a non-zero exit after logging the error so the
app does not run without persistence; update places referencing
settingsstore.MigrateFromGOB and settingsStore.Close to respect the new flag/nil
checks.

---

Outside diff comments:
In `@backend/pkg/plugin/devserver/integration_test.go`:
- Around line 200-208: Replace the inline call
appstate.NewTestService(t).Plugins() with a stored test service variable (e.g.
testSvc := appstate.NewTestService(t)) and pass testSvc.Plugins() into
newGoWatcherProcess; then update the later assertion that currently assumes
$HOME/.omniview/... to instead construct and assert the expected transferred
binary path using the test service's root/plugins location (use the testSvc
variable and its Plugins()/Root() accessor) so the install destination is
derived from the same test service instance used to create the watcher.

---

Duplicate comments:
In `@backend/pkg/plugin/pids.go`:
- Around line 75-78: The ReadFile error handling in the
t.stateRoot.ReadFile(pluginPIDFileName) block treats all errors as "no PID
file"; change it to check os.IsNotExist(err) and return normally only in that
case, and for any other error log the failure (include pluginPIDFileName and the
err value) using the component's logger (e.g., t.logger or the existing logger
in this package) before returning the error so permission/I/O problems aren’t
swallowed.

In `@internal/settings/store/store.go`:
- Around line 21-50: isCorruptionError currently returns true for any
unrecognized open-time error, which can trigger destructive recovery in Open;
update isCorruptionError so it only treats explicit bbolt corruption sentinels
as corruption: check for exported error variables from go.etcd.io/bbolt/errors
(e.g., ErrInvalid, ErrChecksum, ErrVersionMismatch) by using errors.Is, and/or
match the specific bbolt corruption substrings you already list, but change the
default fallback to false (non-corruption) so unknown/transient bolt.Open
failures won't cause rename/recreate in Open; make the change inside
isCorruptionError and ensure callers in Open rely on errors.Is against the
bbolt/errors values or the tightened substring checks.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@backend/pkg/plugin/settings/controller.go`:
- Around line 254-273: tryHydrate may call hydratePlugin multiple times
concurrently for the same plugin; to prevent redundant work, gate hydration per
plugin by adding a per-plugin sync.Once or a "hydrating" state. Modify
controller to maintain a map like onceMap map[string]*sync.Once (or hydrating
map[string]bool) protected by c.mu; in tryHydrate, after checking hydrated and
hasClient, acquire c.mu, get or create the sync.Once for pluginID (or set
hydrating=true), release lock, then invoke once.Do(func(){ c.hydratePlugin(ctx,
pluginID, client, logger) }) (or have hydratePlugin clear hydrating when done).
Ensure any state cleanup and concurrent reads of hydrated still use c.mu to read
the final result before returning.
- Around line 625-631: In SetSettings the timeout path returns without clearing
the pending change, so replicate the same fix used in SetSetting: ensure the
pending entry for the plugin is removed when the timeout case fires (or always
via a defer) and signal any waiters appropriately; locate the Select block using
errCh and the timeout fmt.Errorf("timeout waiting for plugin %q to hydrate",
plugin) and remove/clear p.pending[plugin] (or call the same cleanup helper used
by SetSetting) before returning to avoid orphaned pending state.
- Around line 309-319: The logic that marks pending changes as failed is
duplicated; replace the manual block that locks c.pendingMu, extracts and
deletes c.pendingChanges[pluginID], and loops over pending to send errors with a
call to the existing helper failPendingChanges(pluginID, errors.New("plugin
destroyed")) (or the equivalent signature of failPendingChanges used elsewhere)
so the controller reuses failPendingChanges for consistency; locate the current
snippet referencing c.pendingMu, pending := c.pendingChanges[pluginID],
delete(c.pendingChanges, pluginID), and the loop over p.errCh and replace it
with a single call to failPendingChanges using the same pluginID and error.
- Around line 217-234: The drainPendingChanges loop currently calls
persistCurrentSettings after each successful client.SetSettings; change it to
call persistCurrentSettings once after the loop to reduce I/O: inside
controller.drainPendingChanges, keep the existing pending retrieval and loop
over pending to call client.SetSettings and send p.errCh results as now, but
track a boolean (e.g., anySuccess) that is set true when a SetSettings returns
nil; after the loop, if anySuccess is true call c.persistCurrentSettings(ctx,
pluginID, client, logger) once; preserve the pendingMu usage and deletion
semantics and ensure p.errCh is still written for each pending item.
- Around line 571-577: The timeout branch in SetSetting leaves the pending
change and orphaned errCh active; update the timeout case to remove the pending
change and clean up the channel so the caller's view matches reality: locate the
logic where SetSetting adds an entry to pendingChanges (and the corresponding
errCh) and in the time.After 30s case delete the pendingChanges[plugin] (or call
the existing helper used by drainPendingChanges to remove entries), close or nil
out the errCh to avoid orphaned buffered sends, then return the timeout error;
ensure this mirrors how drainPendingChanges normally clears entries so no
double-delete/race occurs.

---

Duplicate comments:
In `@cmd/omniview-plugin-dev/devinfo.go`:
- Around line 54-69: The expandTilde function currently swallows os.UserHomeDir
errors; change its signature (expandTilde) to return (string, error), and when
os.UserHomeDir() fails return the error instead of the raw "~" path; update both
branches (path == "~" and path[:2] == "~/") to propagate the error, and then
update all callers to handle the returned error (mirroring
internal/appstate/resolve.go's expandAndValidate behavior) so failures are
reported consistently.

In `@internal/appstate/appstate.go`:
- Around line 155-158: The call to s.osRoot.Close() currently discards its error
while other close errors are aggregated; capture the error returned by
s.osRoot.Close() and merge it into the same aggregated error used for the other
close operations (using the project's existing aggregation approach, e.g.,
append to the existing err variable, errors.Join, or multierror) before calling
releaseLock(s.lock), ensuring s.osRoot.Close() uses the same error-handling
pattern as the other closers.

In `@internal/appstate/scoped.go`:
- Around line 152-163: The ResolvePath method currently masks validation
failures by returning s.path when validate(name) fails; change ResolvePath to
return (string, error) instead of just string so callers can detect invalid
names—update ScopedRoot.ResolvePath to return an error when validate(name) fails
(including the original validation error) and return the resolved filepath when
valid; update all callers that use ScopedRoot.ResolvePath (e.g.,
fsnotify/lumberjack integration sites) to handle the error, or if you prefer not
to change the API, at minimum emit a clear warning/log when validate(name) fails
(using the existing logger) before returning s.path so misuse is visible during
development.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@backend/pkg/plugin/settings/controller.go`:
- Around line 637-642: pendingChange.settings is storing the caller's settings
map by reference which can be mutated after queuing; before appending to
c.pendingChanges[plugin] in the SetSettings flow, allocate a new map and copy
all key/value pairs from the incoming settings into it (e.g., create cloned :=
make(map[string]string, len(settings)); for k,v := range settings { cloned[k]=v
}) and store cloned in the pendingChange struct instead of settings so the
queued write uses an immutable snapshot; keep using c.pendingMu as-is to protect
the append.

In `@cmd/omniview-plugin-dev/devinfo.go`:
- Around line 88-90: Validate the pluginID before using it in filepath.Join to
prevent path traversal: reject any pluginID containing path separators ("/" or
"\"), "." or ".." path components, or any characters that would change the path
scope, and return an error; update the creation and removal sites that use
pluginID (e.g., the block that sets dir := filepath.Join(stateDir, "plugins",
pluginID) and the other uses noted around lines 132-137) to call this validation
first and only proceed to os.MkdirAll(dir, 0755) or subsequent file operations
when the pluginID passes validation or use a safe scoped API; ensure the
validation function name (e.g., isValidPluginID or sanitizePluginID) is
referenced and used wherever pluginID is joined into paths.

In `@internal/appstate/appstate.go`:
- Around line 188-194: The code builds plugin data paths by joining s.root with
the raw plugin id, which allows ids containing path separators, "..", or
absolute/volume-qualified names to escape the state dir; update the logic in
appstate.go (the code that creates r via newScopedRoot and where
PluginData/PluginStore directories are derived) to first validate and sanitize
id: reject or error on any id containing path separators ("/" or
os.PathSeparator), any "." or ".." path segments, or any
absolute/volume-qualified form, or alternatively derive child roots by resolving
into the already-open plugins root (e.g., get the plugins scoped root and append
a single-name child) before calling newScopedRoot; ensure the checks are applied
in the same places that currently call newScopedRoot (the block that sets
s.pluginDataRoots[id] and the similar block at lines 209-215) so ids cannot
cause directory traversal or escape.

---

Duplicate comments:
In `@backend/pkg/plugin/settings/controller.go`:
- Around line 130-135: The startup path publishes the client into c.clients
before calling hydratePlugin, allowing concurrent readers to call tryHydrate and
perform a second hydration; fix by routing startup hydration through the same
hydrating guard used by tryHydrate — either call tryHydrate(pluginID) instead of
hydratePlugin or set c.hydrating[pluginID]=true before inserting
c.clients[pluginID] and clear it only after hydratePlugin completes (ensure
proper mutex/locking and panic-safe clearing). Apply the same guard-change to
the other startup block that mirrors this logic (the block around the code
referenced for lines 275-292) so both startup paths use the identical hydrating
flag lifecycle.
- Around line 579-595: The timeout branch for queued writes leaves the matching
pendingChange in c.pendingChanges, so later drainPendingChanges or
failPendingChanges can still apply/send on the abandoned errCh; before returning
from the timeout case in the block that creates errCh and appends pendingChange
(and the duplicate block around the 637-652 region), acquire c.pendingMu, remove
the pendingChange entry for the plugin/location (matching the same pendingChange
by comparing settings map or errCh), and then unlock; this ensures you remove
the buffered pendingChange from c.pendingChanges (and close or ignore its errCh
as appropriate) before returning the timeout error so no late hydration will
report success to the original caller.

In `@cmd/omniview-plugin-dev/devinfo.go`:
- Around line 11-15: The file duplicates the state-dir resolution logic and
introduces a separate build-time knob (stateDirOverride); replace the local
resolution with a call to internal/appstate.ResolveRoot() so there is a single
canonical resolver. Import internal/appstate, remove the duplicated
tilde-expansion/home-fallback/ldflags logic around the stateDirOverride global
and any local resolver functions, and use appstate.ResolveRoot() (passing any
necessary context/flags) wherever the code currently computes the state
directory (referencing the stateDirOverride symbol and the local resolver code
between lines ~28-79) so the code delegates to the canonical resolver instead of
mirroring it.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@backend/pkg/plugin/settings/controller.go`:
- Around line 346-348: OnPluginDestroy currently removes the pluginID from
c.hydrated but leaves c.schemaCache[pluginID], causing a memory leak; update the
OnPluginDestroy implementation (the block using c.mu.Lock()/c.mu.Unlock()) to
also delete the entry from c.schemaCache for the given pluginID while holding
the same mutex so the cleanup is atomic and thread-safe (refer to c.hydrated,
c.schemaCache and c.mu in the change).

In `@internal/appstate/appstate.go`:
- Around line 218-242: The PluginStore method returns nil on error which mirrors
the same unsafe pattern as PluginData; change Service.PluginStore to return
(*ScopedRoot, error) instead of *ScopedRoot, return a descriptive error when
validatePluginID fails or newScopedRoot fails (instead of printing to stderr),
update assignments to s.pluginStoreRoots[id] only on success, and update all
call sites to handle the (ScopedRoot, error) return; keep the existing logic
(validatePluginID, locking with s.pluginRootsMu, checking s.pluginStoreRoots
map, calling newScopedRoot) but propagate errors up rather than returning nil.
- Around line 141-178: Add idempotency to Service.Close by introducing a boolean
closed flag (or sync.Once) on the Service and protecting it with the existing
mutex (pluginRootsMu or a dedicated mu) so repeated Close calls return nil
immediately; specifically, check the flag at the start of Service.Close and
return nil if already closed, set the flag to true before performing close
operations on pluginDataRoots, pluginStoreRoots, plugins, logs, rootDir, and
osRoot, and ensure releaseLock(s.lock) still runs once — this prevents
double-closing resources when Close() is called multiple times while keeping the
existing error aggregation via errors.Join.
- Around line 192-216: Service.PluginData currently returns nil on validation or
creation failure which leads to unchecked dereferences in callers
(backend/pkg/plugin/data/controller.go: Get/Set/Delete/Keys using
root.ReadFile/WriteFile/Remove/ReadDir). Change PluginData signature to return
(*ScopedRoot, error) instead of *ScopedRoot, return a non-nil error when
validatePluginID or newScopedRoot fails, and update the pluginDataRoots caching
logic to store only when newScopedRoot succeeds; then update all callers (Get,
Set, Delete, Keys) to handle the returned error (or explicitly check for nil if
you choose to keep the pointer return) before calling
ReadFile/WriteFile/Remove/ReadDir so they don’t panic. Ensure references to
newScopedRoot, pluginDataRoots, and Service.PluginData are updated accordingly.

---

Outside diff comments:
In `@backend/pkg/plugin/settings/controller.go`:
- Around line 411-440: The Values() method currently reads settings from each
SettingsProvider without attempting hydration, which can yield stale data;
update Values to call c.tryHydrate(pluginID) (the same hydration used by
PluginValues/ListSettings/GetSetting) for each plugin before calling
client.ListSettings(), falling back to bboltFallbackSettings only if
hydration/ListSettings still returns nil, so behavior becomes consistent with
other methods that ensure plugins are hydrated first.

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@backend/pkg/plugin/devserver/gowatch.go`:
- Around line 517-519: syncPluginYaml() can fail to open plugin.yaml if the
plugin directory doesn't exist; mirror transferBinary() by ensuring the
destination directory exists before calling gw.pluginsRoot.OpenFile. Update
syncPluginYaml() to compute the plugin directory from gw.pluginID (same base as
dstRelPath), call MkdirAll (or the existing directory-creation helper used by
transferBinary()) for that directory, then proceed to OpenFile for dstRelPath so
metadata copy won't be skipped when the plugin directory was removed or missing.

In `@backend/pkg/plugin/logs/service_wrapper.go`:
- Around line 14-31: The ServiceWrapper currently allows a zero-value with a nil
Ctrl which defers failure to delegated calls; make Ctrl a required invariant by
preventing zero-value usage and validating at construction/startup: add a
constructor NewServiceWrapper(ctrl Controller) that requires a non-nil
Controller and stores it in an unexported field (e.g., ctrl) and/or have
ServiceStartup immediately validate s.Ctrl (or s.ctrl) and return an error if
nil so ServiceStartup and ServiceShutdown no longer rely on type assertions
against a nil interface; update references to ServiceWrapper.ServiceStartup and
ServiceWrapper.ServiceShutdown to use the validated/unexported field.

In `@backend/pkg/plugin/metric/service_wrapper.go`:
- Around line 15-31: The ServiceWrapper currently allows a zero-value Ctrl which
leads to later panics; make Ctrl a required invariant by validating it up front:
add a constructor NewServiceWrapper(ctrl Controller) that returns a
*ServiceWrapper and requires non-nil ctrl, and update ServiceStartup and
ServiceShutdown to check s.Ctrl == nil and immediately return a clear error
(e.g., "nil Controller in ServiceWrapper") instead of silently returning nil;
also remove or discourage direct struct literal usage so the zero value is
invalid. Ensure references to ServiceWrapper, Ctrl, ServiceStartup,
ServiceShutdown, and NewServiceWrapper are updated accordingly.

In `@backend/pkg/plugin/resource/controller.go`:
- Around line 198-205: The current load/persist logic uses the entire
c.connections map so a plugin can resurrect other plugins' connections and we
also persist an unlocked shared map; change load/save to be plugin-scoped by
only reading/writing c.connections[pluginID] under the c.connsMu lock and
passing that snapshot to loadFromLocalStore/saveToLocalStore (use
c.pluginStoreFn(pluginID) to get storeRoot). Concretely: in the block using
c.pluginStoreFn and loadFromLocalStore, acquire c.connsMu, copy
c.connections[pluginID] to a local slice, release the lock, then call
loadFromLocalStore(storeRoot) and merge/replace only that plugin's slice; do the
same for the stop/persist path (take snapshot under c.connsMu and persist only
that snapshot) so no other plugins' state is loaded or saved and races are
avoided.

In `@backend/pkg/plugin/settings/controller_store_test.go`:
- Line 120: The test is passing nil as the context to hydratePlugin; replace the
nil with context.Background() in the hydratePlugin(...) call (ensure the test
imports the context package) so the test mirrors production usage and avoids
passing a nil context to logging or context-aware code paths.

In `@backend/pkg/plugin/settings/controller.go`:
- Around line 431-444: Avoid mutating shared cached entries: in the loop inside
bboltFallbackSettings (where result := make(...), for k, v := range vals { if
cached, ok := schema[k] { ... }) do not assign to cached.Value directly; instead
make a local copy of the cached pkgsettings.Setting (e.g., copy := cached), set
copy.Value = v and store that copy in result[k]. This prevents modifying the
shared schema cache (protected by schemaCache RLock) and eliminates potential
data races or stale reads used by ListSettings or other callers.

In `@cmd/omniview-plugin-dev/builder.go`:
- Around line 78-79: Validate and sanitize b.meta.ID before using filepath.Join
to build installDir: ensure b.meta.ID is not absolute, contains no path
separators ("/" or "\"), and has no ".." segments; reject or normalize invalid
IDs early. After constructing the path with filepath.Join, call filepath.Clean
and verify the resulting installDir is inside the intended stateDir (e.g., use
filepath.Rel to ensure it does not resolve outside stateDir). Apply this check
where installDir and binDir are computed (references: b.meta.ID, installDir,
binDir, filepath.Join) and return an error if validation fails.

In `@internal/appstate/appstate_test.go`:
- Around line 10-16: The test TestNew_CreatesRootDir hard-codes a path using
"/state" which is not OS-agnostic; update the test to construct the root
argument and the expected path with filepath.Join (use filepath.Join(dir,
"state")) when calling New(WithRoot(...)) and when asserting against svc.Root(),
so the test works across platforms and regardless of path normalization in New()
or Root().

In `@internal/appstate/lock.go`:
- Around line 24-29: The releaseLock function currently discards the error from
fl.Unlock(); modify releaseLock(fl *flock.Flock) to check the returned error and
log it at debug/trace level instead of silently ignoring it—call fl.Unlock(),
capture err, and if err != nil emit a debug log (use the project’s logger or log
package) indicating unlock failure and include the error and context (e.g., the
flock.Flock or a descriptive message) while preserving the nil check behavior.

In `@internal/appstate/resolve.go`:
- Around line 36-52: expandAndValidate currently only expands prefixes "~/", so
on Windows a value like "~\\foo" isn't expanded and later fails the
absolute-path check; update expandAndValidate to also handle the backslash
variant by checking either strings.HasPrefix(dir, "~/") OR
strings.HasPrefix(dir, `~\`) (or more robustly check rune 0 == '~' and rune 1 ==
os.PathSeparator) and then use dir[2:] with filepath.Join(home, dir[2:]) just
like the existing "~/ " case; keep the existing "~" equality branch unchanged.

In `@internal/appstate/testing.go`:
- Line 14: The cleanup currently ignores errors from svc.Close() inside
t.Cleanup(func() { svc.Close() }); update the cleanup to capture and
assert/handle the returned error from svc.Close() so teardown failures fail the
test. Replace the anonymous cleanup body to call svc.Close(), check its error
(e.g., using t.Fatalf/t.Fatalf-like assertion or require.NoError from your test
helpers) and report the error with context referencing svc.Close and the
t.Cleanup wrapper so any Close() failure fails the test instead of being
dropped.

In `@internal/bootstrap/bootstrap.go`:
- Around line 150-156: In ServiceShutdown capture the error returned by
b.TelemetrySvc.Shutdown(context.Background()) instead of discarding it and log
it for debugging; update the ServiceShutdown implementation (which already calls
b.PluginManager.Shutdown() and b.TelemetrySvc.Shutdown(...)) to assign the
shutdown error to a variable, and if non-nil call the service logger (e.g.,
b.Logger or the existing logger field on Service) to emit an error message that
includes the error and context like "telemetry shutdown failed"; keep the
function returning nil after logging so shutdown flow is preserved.

In `@internal/settings/service_wrapper.go`:
- Around line 118-130: In GetCategories (loop over store with variables id and
cat) you're currently emitting pkgsettings.Category with ID: cat.ID which can be
empty or stale; change the emitted ID to the canonical map key by using ID: id
when constructing pkgsettings.Category after merging metadata (see variables id,
cat and the pkgsettings.Category literal) so the category ID always comes from
the store key.

In `@internal/settings/store/store.go`:
- Around line 29-46: Add a brief explanatory comment above the substring-based
corruption detection (the loop that iterates over the list of substrings and the
subsequent default return false) noting that matching err.Error() text is
fragile because bbolt error strings may change, and that the conservative
default to return false is intentional to avoid treating transient I/O/timeouts
as corruption; reference the substring list and the strings.Contains loop so
reviewers can locate the change in the corruption-checking function.

In `@plugin_asset_handler.go`:
- Around line 89-99: Normalize and clean the incoming path before performing the
allowlist check and before calling h.stateRoot.ReadFile: take requestedFilename,
URL-decode and run path.Clean (removing any .. segments), then remove the "/_/"
prefix from the cleaned path (or ensure isAllowed is called with
"/"+cleanedPath) and call isAllowed on the cleaned path (with the leading slash)
— if it fails call respondUnauthorized(); only after passing validation strip
the leading slash and pass the cleaned, stripped path to h.stateRoot.ReadFile;
this ensures functions like isAllowed and h.stateRoot.ReadFile operate on the
normalized path and prevents traversal via inputs like
"/_/plugins/a/assets/../../../other.js".

---

Outside diff comments:
In `@backend/diagnostics/logger.go`:
- Around line 43-57: In NewBackendLogger replace the use of path.Join when
building the log file path (the logFile variable) with filepath.Join so
filesystem paths are constructed correctly across platforms; update imports to
use "path/filepath" (and remove or keep "path" only if still used elsewhere) and
ensure logFile := filepath.Join(baseDir, name+".log") is used before passing it
to the lumberjack/logger or any file operations.
- Around line 177-202: Replace use of path.Join with filepath.Join in filesystem
operations: update ReadLog, SearchLog and StartTail to call
filepath.Join(b.logDir, name+".log") instead of path.Join(...). Ensure the file
imports the "path/filepath" package (and removes or keeps "path" only if still
needed elsewhere). Verify the methods ReadLog, SearchLog and StartTail compile
after switching the join function.

In `@backend/pkg/plugin/devserver/helpers_test.go`:
- Around line 165-174: The test helper currently passes nil for the pluginsRoot
to NewDevServerInstance which is safe for tests that never call Start(), but
risks panics if Start() is used later; update the helper (newTestInstance or the
call site in helpers_test.go where NewDevServerInstance is invoked) to accept an
optional pluginsRoot parameter (or default to a fresh temp dir) and pass a
non-nil value (e.g., t.TempDir()) instead of nil so Start() (which calls
newGoWatcherProcess and uses MkdirAll/Remove/OpenFile) won’t dereference nil;
adjust signatures and all call sites accordingly and add a short comment
documenting that Start() is safe when pluginsRoot is provided.

In `@backend/pkg/plugin/devserver/integration_test.go`:
- Around line 276-288: Extract the repeated appstate.NewTestService(t).Plugins()
into a small test helper: add a function newTestPluginsRoot(t *testing.T)
*appstate.ScopedRoot that simply returns appstate.NewTestService(t).Plugins(),
then replace inline calls (e.g., the argument in the newGoWatcherProcess
invocation) with newTestPluginsRoot(t) to improve readability and reduce
duplication; update all tests in this file that call
appstate.NewTestService(t).Plugins() to use the helper.

In `@backend/pkg/plugin/devserver/ports.go`:
- Around line 108-123: The SavePIDs implementation currently discards the error
from pa.stateRoot.WriteFile; change PortAllocator.SavePIDs to surface write
failures by returning an error (i.e., func (pa *PortAllocator) SavePIDs()
error), capture the result of pa.stateRoot.WriteFile(devserverPIDFileName, b,
0644) into err and return that err (or wrap it with context using fmt.Errorf),
and update all callers of SavePIDs to handle the returned error (or, if you
prefer not to change the signature, at minimum log the error via the existing
logger instead of ignoring it). Ensure you reference the symbols
PortAllocator.SavePIDs, pa.stateRoot.WriteFile and devserverPIDFileName when
making edits.

In `@backend/pkg/store/store.go`:
- Around line 35-47: ReadFromGlobalStore currently calls getStoreFile which
opens with os.O_CREATE, causing reads to silently create empty store files;
change this by using a read-specific open that does not set O_CREATE (e.g., add
a getStoreFileForRead(root, store) or add a createIfMissing bool parameter to
getStoreFile and call it with createIfMissing=false from ReadFromGlobalStore),
ensure this read helper returns an error (os.ErrNotExist) when the file is
missing instead of creating it, and keep using gob.NewDecoder on the opened file
and defer Close() as before; update ReadFromGlobalStore to call the new read
helper (or pass the flag) and handle the returned error unchanged.

In `@backend/pkg/store/utils.go`:
- Around line 36-47: DeexpandTilde can panic by slicing path[len(home):] when
path is shorter than home or doesn't start with home; update DeexpandTilde to
first check that home != "" AND that path has prefix home (use
strings.HasPrefix) and that len(path) >= len(home) before slicing, only then
replace the prefix with "~" (e.g., build "~"+path[len(home):]); otherwise return
the original path; add "strings" to imports and keep fallback behavior on
os.UserHomeDir error.

In `@packages/omniviewdev-runtime/src/hooks/connection/useConnection.tsx`:
- Around line 55-59: The success handler is writing the wrong value into the
cache: inside onSuccess(data, { name }) replace the use of the outer
`connection` variable with the actual `data` returned by the mutation when
calling `queryClient.setQueryData(queryKey, ...)` and when updating lists via
`queryClient.setQueriesData(...)` (use `data` to replace the matching item in
the array by id/name). Update the `onSuccess` implementation so
`queryClient.setQueryData(queryKey, data)` stores the updated Connection and the
`setQueriesData` updater maps items to return `data` for the matching connection
and leaves others unchanged.

In `@packages/omniviewdev-runtime/src/hooks/connection/useConnections.ts`:
- Around line 44-57: The onConnectionSync callback and the effect that
subscribes to Events use variables that can change (queryClient, queryKey,
plugin) but their hooks have empty dependency arrays, which risks stale
closures; update the useCallback for onConnectionSync to include queryClient and
queryKey in its dependency array (or derive a stable callback from them), and
update the useEffect to include plugin and onConnectionSync (or memoize
onConnectionSync) in its dependency array so the event subscription is
re-created when plugin or the handler changes; adjust accordingly in
useConnections.ts around the onConnectionSync function and the React.useEffect
that registers Events.On.

In
`@packages/omniviewdev-runtime/src/hooks/networker/useResourcePortForwarder.tsx`:
- Around line 76-132: Remove the dead commented implementations of forward and
close (the commented PortForwardResourceFunction block that calls
StartResourcePortForwardingSession and the commented close block that calls
ClosePortForwardSession and manipulates sessions), since they are superseded by
the mutation-based approach using closeMutation/useMutation and queryClient
invalidation; delete those commented blocks to reduce maintenance noise and keep
references to the active mutation-based flow (closeMutation,
ClosePortForwardSession, queryClient, ALL_SESSIONS_KEY) as the single source of
truth.

In `@packages/omniviewdev-runtime/src/hooks/resource/useResourceGroups.ts`:
- Around line 19-22: The JSDoc above the useResourceGroups hook is a copy-paste
from a settings hook and is misleading; update the comment for the
useResourceGroups export (function/useResourceGroups) to accurately describe
that it fetches and interacts with resource groups from the global resource
provider (intended for resource listing/management in the UI), mention its
purpose and usage briefly, and remove any references to plugin settings or
useSettings so the doc matches the hook's behavior.

In `@packages/omniviewdev-runtime/src/hooks/resource/useResourceTypes.ts`:
- Around line 25-31: The queryKey for the useQuery call in useResourceTypes is
missing connectionID which causes cache collisions; update the queryKey (the
const named queryKey used by useQuery) to include connectionID (e.g., include
connectionID in the array alongside pluginID and 'resources','list') so that
GetResourceTypes(pluginID, connectionID) is cached per-connection and no longer
shares results across different connections.